fs-http: document + pin the middleware contract (R2/R3, no runtime change)#159
Conversation
Goosterhof
added
the
Agent Review Requested
Deploying fs-packages with
|
| Latest commit: |
5d1bc7f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ef7dce7.fs-packages.pages.dev |
| Branch Preview URL: | https://war-room-wr0049-fs-http-r2-r.fs-packages.pages.dev |
Goosterhof
left a comment
Goosterhof
left a comment
There was a problem hiding this comment.
Verified the "no runtime change" claim directly: packages/http/src/http.ts and packages/http/src/guarded.ts are untouched by this diff (only README.md, types.ts, and two spec files change). types.ts's +38 lines are pure JSDoc additions above three unchanged exported signatures (registerRequestMiddleware/registerResponseMiddleware/registerResponseErrorMiddleware) — no type is narrowed, widened, or renamed. Claim holds.
Cross-checked the new middleware-contract.spec.ts (8 tests) and the guarded.spec.ts addition against the actual runtime at HEAD (src/http.ts's bare for (const middleware of X) middleware(arg) loops, src/guarded.ts's synchronous try/catch):
- FIFO / per-instance / non-idempotent-registration / mutation-visibility / sync-loop-completes / async-not-awaited — all match the real control flow.
- The one behavior-relevant new fact —
guarded()only catches a synchronous throw, so an async-bodied middleware's rejected promise is invisible to it and escapes as an unhandled rejection — is correctly modeled and doesn't duplicate the existinghttp.spec.ts§"middleware guarding by default (ADR-0037)" suite (that suite already pins sync throw-isolation +{guard:false}propagation +onMiddlewareErrorrouting; this PR explicitly doesn't re-pin those, and I confirmed no overlap). The new response-path FIFO test also doesn't duplicatehttp.spec.ts:322's existing request-path ordering test — different path, correctly scoped.
Two nits, no blockers/majors:
types.tsJSDoc adds@returns an idempotent unregister— a claim not in the PR's enumerated 8 contract-pin tests (nor in any pre-existinghttp.spec.tstest I could find) that callingunregister()twice is a safe no-op. It happens to be true (array.indexOfreturns-1after the first splice, guard skips), but it's asserted in prose only.- The "does not reuse the response object" test pins axios's own per-request object-identity guarantee, not an fs-http invariant — harmless, but if it ever fails the cause is upstream, not this package.
CI: format/lint/build/typecheck/lint:pkg/test:coverage all green at review time; test:mutation was still running (not failed) — worth a final glance before merge but not a gate concern from this diff.
No blockers/majors. Own PR — capping at COMMENT.
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
dmooibroek
left a comment
dmooibroek
left a comment
There was a problem hiding this comment.
Clean. Docs+tests only, no src behavior change (http.ts/guarded.ts untouched, types.ts additive). One disagreement with the-general's nit fid140: idempotent unregister() IS covered pre-existing at http.spec.ts:433 (double-call, asserts no-throw + stays removed; predates this PR per git log -p) — the nit's premise is wrong.
…ange)
Surveyor M2 (2026-05-13) distilled twelve middleware invariants and
recommended documenting them (R2) and adding contract-pin tests (R3).
Re-verified every invariant against HEAD (fs-http 0.6.0, guard-by-default
ADR-0037), not the pre-0.6.0 report: exactly one — sync-throw-aborts-caller
— is superseded (now loud-swallowed by default, propagates only under
{guard: false}) and is already pinned in http.spec.ts's guard-by-default
suite. The rest hold unchanged.
R2 (docs, no behavior change):
- README §"Middleware contract" — guard-by-default + {guard:false} opt-out,
sync-only/fire-and-forget, the async-rejection gap guard-by-default does
NOT close, FIFO, per-instance scoping, non-idempotency, mutation
visibility + response-object freshness.
- JSDoc on the three HttpService register* signatures (editor-surface
contract).
R3 (tests, additive):
- tests/middleware-contract.spec.ts (8 pins) — response-path FIFO,
per-instance scoping, non-idempotent double-fire, mutation visibility,
response-object-not-reused, sync-loop-before-caller-resume, async body
not awaited, and async-rejection-not-caught-by-guard (leak-safe via a
pre-handled rejected promise).
- guarded.spec.ts +1 unit pin — guarded() is synchronous-only; a
rejected-promise-returning body is not caught (no onError).
Docs + tests only. No source runtime change. Gate: format/lint/build/
typecheck/lint:pkg green; test:coverage 658 passed, exit 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FFY2M2jh9MLDWFwwcSi7Go
c953f6a to
d446a80
Compare
Goosterhof
left a comment
Goosterhof
left a comment
There was a problem hiding this comment.
⚪ Approve-worthy
0 blockers · 0 majors · 0 minors · 2 nits (both prior, already dispositioned) · 0 inline
Round 2 — since c953f6a (our prior review): the only PR-authored commit is d446a80c2, and cross-referencing dmooibroek's bus-side note (patch-id 1602d102 identical across the head bump), this is a pure rebase onto main, zero authored delta. Independently confirmed against this diff: packages/http/src/http.ts and guarded.ts remain untouched, types.ts is JSDoc-only (+38 above unchanged signatures), and the two spec files are additive-only. CI green (ci-passed, check, town-crier/gate).
Our two open nits carry forward unchanged, both already dispositioned in-thread:
packages/http/src/types.ts:99(fid140, idempotent-unregister()claim) — refuted: independently re-verifiedpackages/http/tests/http.spec.ts:433-447("unregistering a non-existent middleware is a no-op") double-callsunregister()and asserts no-throw + stays removed — that predates this PR and is the idempotency pin. dmooibroek's disposition stands.packages/http/tests/middleware-contract.spec.ts:122(fid141, response-object-identity test pins axios's own guarantee, not an fs-http invariant) — informational, no author action required.
No new findings on this head. Reposting as coverage since our last GH-visible review predates the rebase; own PR — capping at COMMENT.
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
…eview nit fid141) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019bM8WgP1qKadymc36wJYCL
There was a problem hiding this comment.
R4 delta d446a80 -> 5d1bc7f: one authored commit, +2 comment lines in middleware-contract.spec.ts L120-121 — documents the response-identity test pins axios's own guarantee (triage upstream on failure). Exactly addresses fid141; no behavior change, no src touch. Priors stand: fid140 refuted (pre-existing coverage http.spec.ts:433), fid141 now documented in-code. Clean.
Resolved since last review: 1.
2 participants
War-room dispatch WR-0049 (Surveyor M2 hardening recs). Documents (R2) and pins (R3) the fs-http middleware invariants. Docs + tests only — no runtime change, no version bump, no CHANGELOG (no release warranted).
Verify-first vs ADR-0037 (0.6.0 guard-by-default)
The source report predates 0.6.0, so each of the 12 invariants was re-verified against HEAD. Result: exactly one was superseded — the old "sync throw aborts the caller" is now "loud-swallowed by default, propagates only under
{guard:false}" (already pinned inhttp.spec.ts's guard-by-default suite). The other 11 hold unchanged.R2 — docs
README.md— new#### Middleware contractsubsection: guard-by-default +{guard:false}opt-out, sync-only/fire-and-forget, FIFO, per-instance scoping, non-idempotency, mutation visibility + response-object freshness, and the one gap guard-by-default does NOT close.types.ts— JSDoc on the threeregister*signatures (flows into the emitted.d.mts).R3 — 8 contract-pin tests (
middleware-contract.spec.ts) + 1 unit pin (guarded.spec.ts)response-path FIFO · per-instance scoping · non-idempotent double-fire · mutation visibility · response-object-not-reused · sync-loop-completes-before-caller-resumes · async-body-not-awaited · async-rejection-not-caught-by-guard. (Throw-isolation was already fully pinned — not duplicated.) Async-rejection tests use pre-handled rejected promises so no floating unhandled rejection poisons the runner.
guard-by-default catches SYNCHRONOUS throws only. An async-bodied middleware whose Promise rejects is not caught by
guarded()and does not reachonMiddlewareError— it still surfaces as an unhandled rejection. Worth carrying into consumer-adoption guidance. Documented in README + JSDoc, pinned twice (unit + integration).Verification
format 166 ✓ · oxlint 0 · build (ESM+CJS) ✓ · typecheck ✓ · coverage exit 0 (658 tests) · lint:pkg attw all 🟢. 4 files, +262, no
srcbehavior change.🤖 Generated with Claude Code